-
Notifications
You must be signed in to change notification settings - Fork 9.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Resources: aws_acm_certificate and aws_acm_certificate_validation #2813
New Resources: aws_acm_certificate and aws_acm_certificate_validation #2813
Conversation
…certificate can be validated and is issued
Removed the |
@bflad what you think? it's possible to merge this PR? |
Agreed we have a new terraform that would benefit from this. |
You caught me. 😄 I'm in the ACM neighborhood right now with #1837 and was actually planning on looking at this right after so I didn't have too much context switching. While that one is waiting for some feedback let me at least provide an initial review for this one. More shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this is AWESOME! Thank you so much for all the effort here! This is really off to a great start (I promise, even with all the comments below 😄 ).
I'm leaving this as a first round of feedback of since it is indeed a Size/XXL
PR and has two resources (we generally prefer one at a time, but I totally get why this is like this). Admittedly, I have not run the acceptance testing yet or given the validation resource a hardcore line-by-line review like the first since I'd like to shore up the core aws_acm_certificate
first.
Please don't hesitate to ask questions or pushback about anything here. Your feedback is just as valuable.
Let me know when this is ready for a round two review. I will keep this high on my priority queue. I am (assuredly among many others) excited to see this ship soon! 🚀
aws/resource_aws_acm_certificate.go
Outdated
}, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"domain_name": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: the &schema.Schema
for all the attributes here are extraneous since Go 1.7, but since we have a lot of older code you probably saw these other places 😄 (important note: the Elem: &schmema.Schema
ones are still necessary)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -0,0 +1,237 @@ | |||
package aws | |||
|
|||
import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: We try to keep the standard packages up top and third party below in the imports. Most of the maintainers run goimports
to automatically organize (and add/remove!) the imports. Its pretty awesome if you haven't tried it, especially as a post-save hook in your editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks for the pointers to goimports
, that helps a lot!
aws/resource_aws_acm_certificate.go
Outdated
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
- I do not think there's a reason to force only
DNS
as the only valid method here. If someone wants to create aEMAIL
validated certificate with this resource (e.g. for later applying thearn
attribute of this resource to other resources), I don't think we should necessarily stop them. - Nitpick: constant is available
acm.ValidationMethodDns
instead of "DNS"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the constant now.
Supporting E-Mail validation will probably add a bit more work and complexity. It's also hard to test.
At a minimum, we'd need to consider cases where domain validation information isn't present to make sure the provider doesn't break if someone tries to use it with EMAIL
.
Since this PR is already pretty big, my initial idea was to add support for E-Mail validation in a separate PR later if there is demand for it. If DNS validation works smoothly, I'd guess most people (unless they can't for some reason) will use it and not bother with E-Mail.
Does that make sense? If you prefer adding E-Mail support right now, I'd also be willing to invest a bit of time to get it in.
aws/resource_aws_acm_certificate.go
Outdated
return | ||
}, | ||
}, | ||
"certificate_arn": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things about this attribute:
- Can we change it to just
arn
to match many of our other resources? ForceNew
is extraneous here withComputed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Renamed to
arn
inacm_certificate
. I leftcertificate_arn
inacm_certificate_validation
for now as I feel it's not quite as obvious whicharn
is meant there. No strong opinion though, happy to change it if you feel otherwise - Removed
ForceNew
aws/resource_aws_acm_certificate.go
Outdated
Computed: true, | ||
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"domain_name": &schema.Schema{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: the &schema.Schema
in this map are also extraneous 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
} | ||
|
||
// Verify the error is what we want | ||
acmerr, ok := err.(awserr.Error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can simplify this with just: if isAWSErr(err, acm.ErrCodeResourceNotFoundException, "")
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks for the pointer to isAWSErr
!
} | ||
} | ||
|
||
data "aws_route53_zone" "zone" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I think the configuration example should include pieces relevant only to this resource itself. The Route 53 bits here don't fit into everyone's situation, would require documentation updates twice potentially, and we already (very nicely!) mention the validation resource above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. If I understand you correctly we leave the more complete example in the docs for validation? I think otherwise it will be hard to follow how these resources work together (e.g. that you should use the certificate_arn
from the aws_acm_certificate_validation
resource and not aws_acm_certificate
if you want to make sure you depend on an issued certificate).
|
||
## Attributes Reference | ||
|
||
The following attributes are exported: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following additional attributes are exported:
(we've had Github issues about the wording previously 😄 ).
We should also add a mention of:
* `id` - The ARN of the certificate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I also removed the attributes reference section from the acm_certificate_validation
docs since that resource doesn't export any additional attributes.
|
||
* `domain_name` - (Required) A domain name for which the certificate should be issued | ||
* `subject_alternative_names` - (Optional) A list of domains that should be SANs in the issued certificate | ||
* `validation_method` - (Required) Which method to use for validation (only `DNS` is supported at the moment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the validation is updated, this should have both DNS
and EMAIL
and probably a shoutout that automated validation is only possible with DNS
at this time 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
```hcl | ||
resource "aws_acm_certificate" "cert" { | ||
domain_name = "example.com" | ||
validation_method = "DNS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tabs and spaces... 😄 Fixed.
…ng constants and utility methods, removing extraneous code
…t for the whole validation flow into per-resource tests
…instead of special attribute
@bflad unless I missed something this should be ready for round two:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to give this a lovely ✅ ! This is a huge addition to the provider and its at the point where we can triage any little things with it merged in. I have a few testing nitpicks but I'll leave that as a post-merge exercise for myself.
@flosell thank you so much! You just made a bunch of people very happy! 😄
make testacc TEST=./aws TESTARGS='-run=TestAccAwsAcmResource'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAcmResource -timeout 120m
=== RUN TestAccAwsAcmResource_emailValidation
--- SKIP: TestAccAwsAcmResource_emailValidation (0.00s)
resource_aws_acm_certificate_test.go:21: Environment variable ACM_CERTIFICATE_ROOT_DOMAIN is not set
=== RUN TestAccAwsAcmResource_dnsValidationAndTagging
--- SKIP: TestAccAwsAcmResource_dnsValidationAndTagging (0.00s)
resource_aws_acm_certificate_test.go:57: Environment variable ACM_CERTIFICATE_ROOT_DOMAIN is not set
=== RUN TestAccAwsAcmResource_certificateIssuingAndValidationFlow
--- SKIP: TestAccAwsAcmResource_certificateIssuingAndValidationFlow (0.00s)
resource_aws_acm_certificate_validation_test.go:16: Environment variable ACM_CERTIFICATE_ROOT_DOMAIN is not set
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 0.040s
export ACM_CERTIFICATE_ROOT_DOMAIN=valid-public-route53-domain.tld
make testacc TEST=./aws TESTARGS='-run=TestAccAwsAcmResource'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsAcmResource -timeout 120m
=== RUN TestAccAwsAcmResource_emailValidation
--- PASS: TestAccAwsAcmResource_emailValidation (18.68s)
=== RUN TestAccAwsAcmResource_dnsValidationAndTagging
--- PASS: TestAccAwsAcmResource_dnsValidationAndTagging (58.55s)
=== RUN TestAccAwsAcmResource_certificateIssuingAndValidationFlow
--- PASS: TestAccAwsAcmResource_certificateIssuingAndValidationFlow (216.07s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 293.343s
This will be a massive help to us. |
This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
Hello, this is great! The example code works fine. But I have one problem. How do I handle creating multiple DNS records for an ACM cert with multiple subject alternative names? All of the facts I need are embedded in the resource's
This results in this output:
Fair enough. But then I am stuck at how to create those three records. My first instinct is to treat this property like a multi-instance resource, and do something like:
But that gives me an error: "value of 'count' cannot be computed" If I tweak things around or even hardcode "count" to the correct value, then I get errors like: "Resource 'aws_acm_certificate.cert' does not have attribute 'domain_validation_options.*.resource_record_type'". I've tried using locals as interstitial calculators, but I can't figure out how to get, say, an array of just the resource_record_name values, given the cert_validation structure. I'm also not able to get this to work using just straight local variables instead of the resource attribute. There just don't seem to be sufficient tools in the interpolation language to manipulate this particular data structure. Not sure if it's easier to fix the resource or the interpolation language. |
This exactly describes the challenge I was experiencing yesterday. I know you can attach multiple ACM certs to an ALB, but it's nice if I can minimize the number of certificate lifecycles to manage. |
Has anyone been able to create multiple I am unable to dynamically lookup the But if i try to access it dynamically in a resource block with count like this:
If i try to output just |
I've found that it works if you use:
And similar for the other fields. |
@daveadams Thanks, that works perfectly! |
it doesnt work i you use variable "apps" {
type = "list"
default = ["foo", "bar"]
}
# dns validation record
resource "aws_route53_record" "live_cert_validation" {
count = "${length(var.apps)}"
name = "${lookup(element(aws_acm_certificate.live.*.domain_validation_options[count.index], 0), "resource_record_name")}"
type = "${lookup(element(aws_acm_certificate.live.*.domain_validation_options[count.index], 0), "resource_record_type")}"
zone_id = "${data.aws_route53_zone.live.*.id[count.index]}"
records = ["${lookup(element(aws_acm_certificate.live.*.domain_validation_options[count.index], 0), "resource_record_value")}"]
ttl = 60
} more weird: it works if i create the certificate in one stack, and the certificate dns records and the validation in the next stack. but all in one stack gives me the following error:
it tried it in a lot of obscure ways, but nothing seems to work, i guess the interpolation comes in play to early here... |
Looks like you have a second "type =" in your route53 definition. You
should just have the one pointing to the domain_validation_options record
type attribute. I'm guessing it's getting created as the wrong type.
…On Wed, Feb 28, 2018 at 11:29 AM, Brian Auron ***@***.***> wrote:
Hello, I'm trying to get this to work for me in (what I think is) a pretty
simple manner:
I've created a domain, a certificate request, and a validation request. I
can see that the CNAME exists as expected, but the certificate request just
sits in Pending Validation status for 72 hours and times out:
resource "aws_acm_certificate" "nexus_certificate" {
domain_name = "${var.module_name}.${var.route53_zone_name}"
validation_method = "DNS"
tags {
Environment = "${var.module_name}-${var.route53_zone_name}"
}
}
resource "aws_acm_certificate_validation" "nexus_certificate_validation"{
certificate_arn = "${aws_acm_certificate.nexus_certificate.arn}"
validation_record_fqdns = ["${aws_route53_record.nexus_cert_validation_record.fqdn}"]
}
resource "aws_route53_record" "nexus_cert_validation_record" {
name = "${aws_acm_certificate.nexus_certificate.domain_validation_options.0.resource_record_name}"
type = "${aws_acm_certificate.nexus_certificate.domain_validation_options.0.resource_record_type}"
zone_id = "${var.route53_zone_id}"
ttl = 60
type = "CNAME"
records = ["${aws_acm_certificate.nexus_certificate.domain_validation_options.0.resource_record_value}"]
}
If anybody has any tips or advice, I'd very much appreciate it.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2813 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAETogsexXn940uyq45yJVl-47XmzbqAks5tZYzJgaJpZM4RP30U>
.
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
This is a second draft trying to implement automated ACM certificate issuing based on my suggestions in #2418. It's similar to PR #2801 but uses a resource instead of a data source to implement polling for certificate issuing as a data source implementation seems to have a few pitfalls for the users.
Here is what it does:
aws_acm_certificate
resource that requests a new certificate and returns oncevalidationOptions
(the information what needs to be done to validate ownership of the domain) are available (for some reason, those aren't immediately available). Only supports DNS validation at this pointaws_acm_certificate_validation
resource that does some basic sanity checks and then waits for the given certificate to be issuedCurrent Limitations:
AMAZON_ISSUED
: Importing existing certificate data could be added later if there is demand for itTest considerations:
Acceptance Tests need a publicly resolvable Route53 zone to make certificate validation work. To run tests, set the
ACM_CERTIFICATE_ROOT_DOMAIN
environment variable to the domain name of the Route53 zone. The test will use data-providers to look up the zone-id and add CNAMES to allow ACM to validate ownership of the domain:Here are a few things I'm unsure about:
subject_alternative_names
to contain values when requesting a certificate butDescribeCertificate
always includes thedomain_name
insubject_alternative_names
. I'm currently filtering this so terraform doesn't detect this as a change. Is there a better way to do this?validation_method
property? It currently only allowsDNS
as a value so we could do without. However, it probably makes adding new validation methods easier